cmd/codex_hooks.go (2)

309-333: 💤 Low value

Ownership heuristic is sound; consider documenting it.

isLumenCodexSessionStartCommand recognises a Lumen-owned hook when the launcher path includes /scripts/run.sh or /scripts/run.cmd and the parent dir (case-insensitive) starts with lumen. Combined with the explicit expectedCommand and statusMessage checks in isOwnedLumenCodexSessionStartCommand, this correctly preserves foreign hook session-start lumen commands rooted elsewhere (covered by TestMergeCodexSessionStartHook_PreservesForeignSessionStartCommands). Worth a short doc comment so the "starts with lumen" heuristic isn't accidentally broadened or tightened later.

🤖 Prompt for AI Agents

Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.

In `@cmd/codex_hooks.go` around lines 309 - 333, Add a short doc comment above
isLumenCodexSessionStartCommand and isOwnedLumenCodexSessionStartCommand that
explains the ownership heuristic: that isLumenCodexSessionStartCommand checks
for launcher paths ending with "/scripts/run.sh" or "/scripts/run.cmd" and then
verifies the parent directory name (case-insensitive) starts with "lumen", and
that isOwnedLumenCodexSessionStartCommand additionally allows an exact
expectedCommand match or a codex-specific statusMessage match before falling
back to the heuristic; include the rationale that this preserves foreign "hook
session-start lumen" commands rooted elsewhere and warn not to broaden or
tighten the "starts with lumen" check without updating tests.

---

228-247: 💤 Low value

TOML "comment splitter" doesn't handle multiline strings.

splitTomlLineComment correctly handles single-line '…' / "…" plus backslash escaping inside "…", but it has no notion of TOML multiline strings ("""…""", '''…'''). A # inside a multiline literal that spans lines could be misread as a comment if the opening triple-quote is on a prior line. In practice Codex's config.toml is unlikely to contain such constructs around the [features] block, so this is fine for the current scope — worth a one-line comment scoping the parser's intent (per-line, no multiline string awareness) so future maintainers don't repurpose it elsewhere.

🤖 Prompt for AI Agents

Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.

In `@cmd/codex_hooks.go` around lines 228 - 247, The splitTomlLineComment function
does not understand TOML multiline string delimiters ('''...''' and """...""")
and can misinterpret a '#' inside a multiline string that began on a prior line
as a comment; add a clear one-line doc comment immediately above
splitTomlLineComment stating that this parser is per-line only and intentionally
does not handle TOML multiline strings (triple-quoted literals) so future
maintainers won't reuse it for multiline-aware parsing, or alternatively
implement multiline detection if you want full TOML compliance.

cmd/codex.go (2)

307-328: ⚡ Quick win

Surface "codex CLI missing" early in install, like doctor does.

runCodexDoctor cleanly maps errors.Is(err, exec.ErrNotFound) from mcp get to "MCP lumen: unavailable: codex command not found". ensureCodexMCP does not — when the codex binary is missing, the mcp get failure silently falls through to mcp add, which also fails with ErrNotFound, and the user sees add Codex MCP server: exec: "codex": executable file not found in $PATH. By that point hooks/skills/feature-flag are already installed, which is fine, but the error message obscures the actual cause. A small early-exit on errors.Is(err, exec.ErrNotFound) here would give the same clear UX as doctor and avoid the redundant mcp add attempt.

♻️ Suggested early-exit

 func ensureCodexMCP(ctx context.Context, paths codexPaths, runner commandRunner, stdout, _ io.Writer) error {
 	out, err := runner.Run(ctx, "codex", "mcp", "get", "lumen")
 	if err == nil && codexMCPOutputMatches(out, paths) {
 		_, _ = fmt.Fprintln(stdout, "Codex MCP server lumen already configured")
 		return nil
 	}
 	if err == nil {
 		if _, removeErr := runner.Run(ctx, "codex", "mcp", "remove", "lumen"); removeErr != nil {
 			return fmt.Errorf("remove mismatched Codex MCP server: %w", removeErr)
 		}
 		if _, addErr := runner.Run(ctx, "codex", "mcp", "add", "lumen", "--", paths.launcher, "stdio"); addErr != nil {
 			return fmt.Errorf("add Codex MCP server: %w", addErr)
 		}
 		_, _ = fmt.Fprintln(stdout, "Updated Codex MCP server lumen")
 		return nil
 	}
+	if errors.Is(err, exec.ErrNotFound) {
+		return fmt.Errorf("codex command not found in PATH; install the Codex CLI and re-run `lumen codex install`")
+	}
+	if !codexMCPGetOutputReportsMissing(out) {
+		return fmt.Errorf("query Codex MCP server: %w", err)
+	}
 	if _, addErr := runner.Run(ctx, "codex", "mcp", "add", "lumen", "--", paths.launcher, "stdio"); addErr != nil {
 		return fmt.Errorf("add Codex MCP server: %w", addErr)
 	}
 	_, _ = fmt.Fprintln(stdout, "Added Codex MCP server lumen")
 	return nil
 }

If you take this, double-check TestRunCodexInstall_WritesHookAndAddsMCPWhenMissing — it currently uses getErr: exec.ErrNotFound to simulate "no server yet" rather than "no CLI". Switching that fixture to a non-ErrNotFound error or to codexMCPGetOutputReportsMissing-style stdout would keep the test's intent intact.

🤖 Prompt for AI Agents

Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.

In `@cmd/codex.go` around lines 307 - 328, ensureCodexMCP currently treats a
missing codex binary the same as other runner errors and falls through to
attempting mcp add; update ensureCodexMCP to check the error returned by the
first runner.Run(ctx, "codex", "mcp", "get", "lumen") and if errors.Is(err,
exec.ErrNotFound) return a clear, early error (same UX as runCodexDoctor: e.g.
"MCP lumen: unavailable: codex command not found") instead of proceeding to mcp
add; locate this logic in ensureCodexMCP, use the existing runner.Run result and
codexMCPOutputMatches to distinguish the paths, and adjust any tests that
assumed getErr == exec.ErrNotFound to simulate a different "no server"
condition.

---

599-602: 💤 Low value

Ownership detection couples to two literal SKILL.md headings.

lumenSkillsDirLooksOwned requires the exact strings # Lumen Doctor and # Lumen Reindex to consider the destination "installer-managed". If those headings are ever edited (refactor, branding, frontmatter rearrangement), the installer would silently treat its own previous output as user-owned and refuse to update or uninstall it. Consider keying ownership off the .lumen-skills-source marker alone (which copyCodexSkills already writes) and reserving the heading check as a redundant signal, so renames to SKILL.md content don't strand existing users.

🤖 Prompt for AI Agents

Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.

In `@cmd/codex.go` around lines 599 - 602, The ownership check in
lumenSkillsDirLooksOwned currently requires exact SKILL.md headings and is
fragile; change lumenSkillsDirLooksOwned to first check for the presence of the
marker file that copyCodexSkills writes (the ".lumen-skills-source" file in the
given path) and return true if that marker exists, and only fall back to the
current fileContains heading checks as a redundant signal; update
lumenSkillsDirLooksOwned (and any uses of fileContains within it) to prefer the
marker-based test so renaming/editing SKILL.md headings won't break installer
ownership detection.

cmd/codex_test.go (1)

50-87: 💤 Low value

Launcher assertions are Unix-shaped; consider GOOS-gating or a build tag if Windows testing is added.

makeCodexPluginRoot only creates scripts/run.sh, and tests like this one assert paths.launcher == filepath.Join(pluginRoot, "scripts", "run.sh"). On Windows, codexLauncherPath returns the run.cmd form, so these assertions would fail. Currently, Go tests only run on ubuntu-latest in CI, so this isn't an issue in practice. However, if Windows testing is added to the Go test matrix, gate these assertions with if runtime.GOOS == "windows" { t.Skip(...) } or compute the expected launcher via codexLauncherPath(pluginRoot) instead of hardcoding run.sh.

🤖 Prompt for AI Agents

Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.

In `@cmd/codex_test.go` around lines 50 - 87, TestCodexPathsFromEnv hardcodes a
Unix launcher path which will fail on Windows; change the assertion to compute
the expected launcher using codexLauncherPath(pluginRoot) (or skip the assertion
on windows via runtime.GOOS == "windows") so it matches the platform-specific
value returned by resolveCodexPaths; update the test (TestCodexPathsFromEnv) to
call codexLauncherPath(pluginRoot) for the expected launcher (or conditionally
t.Skip) and keep other assertions the same.

Fix all unresolved CodeRabbit comments on this PR:

• Push a commit to this branch (recommended)
• Create a new PR with the fixes

cmd/codex_hooks.go (1)

351-367: ⚠️ Potential issue | 🟠 Major | ⚡ Quick win

Require launcher filename boundary checks when classifying Lumen-owned commands.

Line 356 + Line 381 currently allow prefix matches (e.g., /scripts/runner), which can misclassify user hooks as Lumen-owned and remove them.

🐛 Proposed fix

 func isLumenCodexSessionStartCommand(command string) bool {
 	normalized := strings.ReplaceAll(command, `\`, "/")
 	if !strings.Contains(normalized, "hook session-start lumen") {
 		return false
 	}
-	root, ok := lumenLauncherRoot(normalized, "/scripts/run")
+	root, ok := lumenLauncherRoot(normalized, "/scripts/run.sh")
 	if !ok {
-		root, ok = lumenLauncherRoot(normalized, "/scripts/run.sh")
+		root, ok = lumenLauncherRoot(normalized, "/scripts/run.cmd")
 	}
 	if !ok {
-		root, ok = lumenLauncherRoot(normalized, "/scripts/run.cmd")
+		root, ok = lumenLauncherRoot(normalized, "/scripts/run")
 	}
 	if !ok {
 		return false
 	}
 	return strings.HasPrefix(strings.ToLower(filepath.Base(root)), "lumen")
 }

 func lumenLauncherRoot(command, suffix string) (string, bool) {
 	idx := strings.Index(command, suffix)
 	if idx < 0 {
 		return "", false
 	}
+	end := idx + len(suffix)
+	if end < len(command) {
+		switch command[end] {
+		case ' ', '\t', '"', '\'':
+		default:
+			return "", false
+		}
+	}
 	root := strings.Trim(command[:idx], `"' `)
 	if root == "" {
 		return "", false
 	}
 	return root, true
 }

#!/bin/bash
set -euo pipefail

# Inspect launcher ownership detection and root extraction implementation.
rg -n -C4 'isLumenCodexSessionStartCommand|lumenLauncherRoot|/scripts/run' cmd/codex_hooks.go

# Confirm there is no explicit regression test for "/scripts/runner"-style false positives.
rg -n -C2 'runner|runbook' cmd/codex_hooks_test.go

Also applies to: 380-389

🤖 Prompt for AI Agents

Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.

In `@cmd/codex_hooks.go` around lines 351 - 367, The current classification in
isLumenCodexSessionStartCommand and lumenLauncherRoot allows prefix matches like
"/scripts/runner"; update the matching so launcher filename checks require
filename boundaries and exact matches: change calls that search for
"/scripts/run", "/scripts/run.sh", "/scripts/run.cmd" to verify the matched
substring is followed only by a path separator or end-of-string (or use
path.Base/EqualFold on the extracted filename), and replace the loose
strings.HasPrefix(strings.ToLower(filepath.Base(root)), "lumen") check with a
case-insensitive exact comparison (e.g., strings.EqualFold(filepath.Base(root),
"lumen")) so only true Lumen-owned launchers are classified. Ensure these
changes touch isLumenCodexSessionStartCommand and lumenLauncherRoot usage.

cmd/codex_test.go (1)

320-340: ⚡ Quick win

Convert this multi-scenario test to table-driven form.

This test exercises several cases and is easier to maintain as a table-driven test.

Refactor sketch

 func TestCodexMCPOutputMatchesRequiresCommandAndArgs(t *testing.T) {
 	paths := codexPaths{launcher: "/repo/lumen/scripts/run"}
-	if !codexMCPOutputMatches([]byte("transport: stdio\ncommand: /repo/lumen/scripts/run\nargs: stdio\n"), paths) {
-		t.Fatal("expected command and args output to match")
-	}
-	...
+	tests := []struct {
+		name string
+		out  []byte
+		want bool
+	}{
+		{"text match", []byte("transport: stdio\ncommand: /repo/lumen/scripts/run\nargs: stdio\n"), true},
+		{"json match", []byte(`{"command":"/repo/lumen/scripts/run","args":["stdio"]}`), true},
+		{"missing command", []byte("transport: stdio\nnote: /repo/lumen/scripts/run\n"), false},
+		{"missing args", []byte("command: /repo/lumen/scripts/run\nnote: stdio\n"), false},
+	}
+	for _, tc := range tests {
+		t.Run(tc.name, func(t *testing.T) {
+			if got := codexMCPOutputMatches(tc.out, paths); got != tc.want {
+				t.Fatalf("got %v, want %v", got, tc.want)
+			}
+		})
+	}
 }

As per coding guidelines, "Use table-driven tests for multiple test cases in Go."

🤖 Prompt for AI Agents

Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.

In `@cmd/codex_test.go` around lines 320 - 340, Refactor
TestCodexMCPOutputMatchesRequiresCommandAndArgs into a table-driven test: define
a slice of test cases (struct with name, input []byte, paths codexPaths, want
bool) covering each existing scenario, then loop over cases with t.Run(tc.name,
func(t *testing.T){ if got := codexMCPOutputMatches(tc.input, tc.paths); got !=
tc.want { t.Fatalf("... expected %v got %v", tc.want, got) } }), keeping the
same inputs and assertions; reference the existing test function name
TestCodexMCPOutputMatchesRequiresCommandAndArgs and the helper
codexMCPOutputMatches/codexPaths to locate code to change.

cmd/codex.go (2)

167-180: ⚡ Quick win

Use defer-based file handle cleanup in temp-write paths.

Both temp-file writers close handles manually across branches; switch to a defer pattern for handle cleanup consistency and simpler error paths.

As per coding guidelines, "Always defer resource cleanup for database and file handles using defer Close()."

Also applies to: 229-242

🤖 Prompt for AI Agents

Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.

In `@cmd/codex.go` around lines 167 - 180, The temp-file write path manually
closes and removes the file in multiple branches (using tmp, tmpName after
os.CreateTemp) — replace the manual closes with a defer-based cleanup:
immediately after successful os.CreateTemp assign tmpName := tmp.Name() and
defer func() { _ = tmp.Close(); if shouldRemove { _ = os.Remove(tmpName) }() }
(or similar) so the file handle is always closed; then update the error branches
for Write and Close to set shouldRemove appropriately (or remove only on error)
instead of duplicating tmp.Close()/os.Remove calls; apply the same defer-based
pattern to the other temp-write block using the same tmp/tmpName/Write/Close
variables.

---

330-334: ⚡ Quick win

Avoid false MCP mismatches for equivalent launcher paths.

Strict string equality can mark symlink-equivalent paths as mismatched, causing unnecessary remove + add churn.

Proposed change

 func codexMCPOutputMatches(out []byte, paths codexPaths) bool {
 	command, args := parseCodexMCPOutput(out)
-	launcher := strings.ReplaceAll(paths.launcher, `\`, "/")
-	return command == launcher && len(args) == 1 && args[0] == "stdio"
+	if len(args) != 1 || args[0] != "stdio" {
+		return false
+	}
+	return sameFileTree(command, paths.launcher)
 }

🤖 Prompt for AI Agents

Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.

In `@cmd/codex.go` around lines 330 - 334, codexMCPOutputMatches is comparing the
launcher path string-for-string which will treat
symlink/relative/path-separator/case-equivalent paths as different; instead
canonicalize both the extracted command and paths.launcher before comparing:
call filepath.Abs + filepath.EvalSymlinks + filepath.Clean on both command and
paths.launcher (falling back to Clean if Abs/EvalSymlinks error), normalize
separators with filepath.ToSlash (or ToSlash on both), and on Windows use
strings.EqualFold when comparing; then check equality and the args as before.
Ensure you update references to parseCodexMCPOutput, command, and paths.launcher
in the codexMCPOutputMatches function.

Fix all unresolved CodeRabbit comments on this PR:

• Push a commit to this branch (recommended)
• Create a new PR with the fixes